Adopt new NodeJS runtime toolchain #2330
Conversation
|
|
nice, verified this fixes this case too #2328 |
guw
force-pushed
the
use_runtime_toolchain
branch
from
1390f08 to
3eb24e8
Compare
|
I am left with the following issue when building: |
guw
force-pushed
the
use_runtime_toolchain
branch
from
3eb24e8 to
f997478
Compare
js/private/js_image_layer.bzl
Outdated
| toolchains = [ | ||
| tar_lib.toolchain_type, | ||
| "@rules_nodejs//nodejs:toolchain_type", | ||
| config_common.toolchain_type("@rules_nodejs//nodejs:toolchain_type", mandatory = False), # for executing Node in our actions (must not be mandatory because it only available in 'exec' config) |
There was a problem hiding this comment.
@fmeum I had to make this non-mandatory. Can I get your opinion whether that is plausible?
There was a problem hiding this comment.
I haven't been able to find where these toolchains are used. Could they just be dropped?
There was a problem hiding this comment.
I was assuming they are needed because _node_tool and js_binary needs them. Are you saying it can be removed here?
js/private/js_image_layer.bzl
Outdated
| toolchains = [ | ||
| tar_lib.toolchain_type, | ||
| "@rules_nodejs//nodejs:toolchain_type", | ||
| config_common.toolchain_type("@rules_nodejs//nodejs:toolchain_type", mandatory = False), # for executing Node in our actions (must not be mandatory because it only available in 'exec' config) |
There was a problem hiding this comment.
I haven't been able to find where these toolchains are used. Could they just be dropped?
| ), | ||
| "_current_node": attr.label( | ||
| default = "@nodejs_toolchains//:resolved_toolchain", | ||
| "_node_tool": attr.label( |
There was a problem hiding this comment.
This attribute could be replaced with @rules_nodejs//nodejs:toolchain_type as that matches on the exec platform.
There was a problem hiding this comment.
What is the recommended approach?
I could replace:
nodeinfo = ctx.attr._node_tool[platform_common.ToolchainInfo].nodeinfo
....
with:
nodeinfo = ctx.toolchains["@rules_nodejs//nodejs:toolchain_type"].nodeinfo
if hasattr(nodeinfo, "node"):
node_exec = nodeinfo.node
else:
# TODO(3.0): drop support for deprecated toolchain attributes
node_exec = nodeinfo.target_tool_path
ctx.actions.run(
inputs = inputs,
arguments = [splitter.path],
unused_inputs_list = unused_inputs,
outputs = splitter_outputs,
executable = node_exec,
progress_message = "Computing Layer Groups %{label}",
mnemonic = "JsImageLayerGroups",
)
But how do I control selection based on exec then?
There was a problem hiding this comment.
Oh so, nodeinfo = ctx.toolchains["@rules_nodejs//nodejs:toolchain_type"].nodeinfo would always match the execution platform?
There was a problem hiding this comment.
Yes, since that's what we ask the toolchains registered for this type to do. That's the idiomatic approach to use tools.
There was a problem hiding this comment.
Just keep in mind that the ctx.actions.run call should set the toolchain parameter to the toolchain type used in the action (if there is a single one, which is the case here).
js_binary should use the new runtime toolchain to avoid execution toolchain being leaked into target environments (eg., js_image_oci) See bazel-contrib/rules_nodejs#3854
guw
force-pushed
the
use_runtime_toolchain
branch
from
f997478 to
6e15cc8
Compare
3 participants
js_binaryshould use the new runtime toolchain to avoid execution toolchain being leaked into target environments (eg.,js_image_oci)See bazel-contrib/rules_nodejs#3854
Depends on:
Changes are visible to end-users: yes
Test plan